Proposed "snappy" vs "x-snappy-framed" content-type confusion #12825

skandragon · 2025-04-11T06:52:44Z

Description

The collector http compression and decompression logic uses "snappy" incorrectly. The content-type "snappy" should be the block ("non-framed") version, while "x-snappy-framed" should be the framed one. Unfortunately, the collector code used framed compression and decompression and called it "snappy."

This corrects this in two ways:

Add x-snappy-framed as a content-type for both incoming and outgoing compression.
Add a (hopefully temporary) hack that will, when decompressing content-type "snappy", peek into the data, and see if the 10 byte snappy-framed header is present. If it is, uncompress it using framed, otherwise use the block decompression.

In contrast to other proposals in the linked issue, this feels like a clean path forward as warnings about using "snappy" in the docs can be added, and people can either not change (in which case it will just start working using the block format) or change, in which case it will use framed.

Link to tracking issue

Fixes #10584 by providing a way forward

Testing

Test correct and incorrect configuration of both "snappy" and "x-snappy-framed" content-types.
Add a test for content-type "snappy" but framed data.

codecov · 2025-04-11T07:31:25Z

Codecov Report

Attention: Patch coverage is 80.43478% with 9 lines in your changes missing coverage. Please review.

Project coverage is 91.42%. Comparing base (568c3ce) to head (db48f79).
Report is 104 commits behind head on main.

Files with missing lines	Patch %	Lines
config/confighttp/compression.go	76.00%	4 Missing and 2 partials ⚠️
config/confighttp/compressor.go	85.00%	2 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (80.43%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12825      +/-   ##
==========================================
- Coverage   91.44%   91.42%   -0.02%     
==========================================
  Files         487      487              
  Lines       26848    26892      +44     
==========================================
+ Hits        24551    24586      +35     
- Misses       1814     1820       +6     
- Partials      483      486       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dmathieu · 2025-04-11T07:34:23Z

Please fix the lint issues and add a changelog entry.

bogdandrutu

Please link to what is the "authority" that defines the "correct" usage? I am not sure I understand who defines corectness here.

bogdandrutu · 2025-04-11T16:51:26Z

.chloggen/snappy-done-right.yaml

+  the "block" compression variant of "snappy".  This change allows the collector to:
+  - The server endpoints will now accept "x-snappy-framed" as a valid
+    content-encoding.
+  - Client compression type "snappy" will now compress to the "block" variant of snappy


Please don't do this, since as @jpkrohling explained, this will break existing servers that are not updated.

First we need to add support for both on both server/client. The "hack" for the server can also go now. Then wait for few versions until you can change the client.

What specific behavior do we want here? I tried to avoid breaking receiving as right now that felt like the pain point. Should we add x-snappy-framed as a type first, and then at some point later switch the compression to use non-framed for "snappy"?

If I changed the PR to do this, would this satisfy the path forward and start the ball rolling?

Your PR avoids breakage in the situation where the receiver is updated but not the sender (and that's great!), but not in the opposite scenario: if the sender is updated but not the receiver, the sender will switch to block mode, and the receiver will not accept it. Of course, the sender can "just" update their config to explicitly switch back to framed mode, but this could still be a source of breakage.

Bogdan's suggestion is to add the protocol detection in the receiver (and the explicit framed mode) now, but to wait a few versions before switching "snappy" to use the block mode. This means that breakage will only occur if the sender gets both updates and the receiver gets none, which is less likely.

I think one additional possibility, to ease the transition and minimize how much code change needs to be postponed, would be to put the switch of snappy from framed to block mode behind an Alpha (off by default) feature gate, and add a warning log in confighttp when "snappy" is used with the gate disabled. Then later, we will only need to switch it to Beta (this is where the breaking change would be). What do you think?

I like this suggestion. For the components that need to use snappy encoding/decoding correctly, we just need to document that the new feature gate is required for them to work.

I don't see anybody else requiring this besides the prometheus remote-write receiver, which is not even an alpha component yet, so it sounds reasonable

+1 with all the comments.

bogdandrutu · 2025-04-11T16:51:42Z

.chloggen/snappy-done-right.yaml

+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: enhancement


Definitely not enhancement, this is a complete breaking change.

Should we keep it as enhancement if we switch the strategy to using feature flags?

I think it would no longer be a breaking change if we delay the client changes (unless there are bugs in the protocol detection).

skandragon · 2025-04-11T19:45:49Z

Please link to what is the "authority" that defines the "correct" usage? I am not sure I understand who defines corectness here.

There isn't an authority, except that the common usage is "snappy means block format" and "x-snappy-framed" means the streaming friendly framed format. The only real authority I could find is that in the spec for the framed format, it specifically defines this:

2. File type identification

The following identifiers for this format are recommended where appropriate.
However, note that none have been registered officially, so this is only to
be taken as a guideline. We use "Snappy framed" to distinguish between this
format and raw Snappy data.

  File extension:         .sz
  MIME type:              application/x-snappy-framed
  HTTP Content-Encoding:  x-snappy-framed

(see https://github.com/google/snappy/blob/main/framing_format.txt)

ArthurSens · 2025-04-11T21:44:07Z

Hi @skandragon , thanks for working on this! I'm one of the codeowner of Prometheus Remote Write Receiver 👋

@perebaj and I were working on getting the receiver to alpha stage until we got blocked for several weeks due to prw requests failing to be decoded via httpconfig(open-telemetry/opentelemetry-collector-contrib#38899), until I randomly bumped into your PR today.

Now that I've read your PR and the linked issue, things are making sense! One suggestion to make sure your PR works for the remote-write receiver is trying to build the collector-contrib, but updating the collector dependency to your commit SHA and verifying that it works. See open-telemetry/opentelemetry-collector-contrib#38899 (comment) for instructions.

I'll try to find some time next week to help out here

ArthurSens

👋 -- I've just tested the code changes in open-telemetry/opentelemetry-collector-contrib#39446, it works :)

I agree that it's not that simple to just change things like has been said already. How do you feel about the suggestion of using feature flags instead @skandragon? We could abandon the logic that detects the format by reading the first bytes of the sequence, which also helps with code readability in my opinion 🤔

ArthurSens · 2025-04-15T21:41:42Z

.chloggen/snappy-done-right.yaml

+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: enhancement


Should we keep it as enhancement if we switch the strategy to using feature flags?

jade-guiton-dd · 2025-04-16T12:32:11Z

We could abandon the logic that detects the format by reading the first bytes of the sequence, which also helps with code readability in my opinion 🤔

I think removing the protocol detection logic would cause issues once we set the feature flag to beta; you wouldn't be able to update the client before the server.

bogdandrutu · 2025-04-17T17:39:14Z

@jade-guiton-dd @skandragon @ArthurSens does this apply to gRPC as well?

jade-guiton-dd · 2025-04-18T09:54:30Z

It looks like configgrpc uses the "snappy" gRPC compressor from https://github.com/mostynb/go-grpc-compression, which defers to https://github.com/golang/snappy. Not sure which version of the protocol this is implementing though.

It seems gRPC doesn't use the Content-Encoding header, and has its own compression header grpc-encoding, so I'm not sure if there are established conventions for that.

perebaj

Maybe would be nice to update the README with the x-snappy-framed compression type

https://github.com/cardinalhq/opentelemetry-collector/blob/db48f79d353027fe86ee94cd20e2788006fcd5f1/config/confighttp/README.md#L26

skandragon · 2025-04-28T19:19:28Z

Hi @skandragon , thanks for working on this! I'm one of the codeowner of Prometheus Remote Write Receiver 👋

Howdy!

Perhaps we should chat a bit. I want to use the remote write receiver as part of our managed collector framework (cardinalhq.io for reference) where we want to make it easy to migrate from a prometheus shop to an Otel data flow. We are adding scraping configs, but that seems less useful than just getting the firehose from prometheus directly.

Let's chat and see if we can help develop the receiver.

ArthurSens · 2025-04-28T20:06:34Z

That would be great!

We're currently developing it as part of a mentorship, @perebaj is doing most of the heavy work here :)
We discuss the development of all Prometheus-related components in CNCF's slack, specifically in the #otel-prometheus channel. Come say hi 👋

perebaj · 2025-04-28T20:22:41Z

Hi @skandragon , thanks for working on this! I'm one of the codeowner of Prometheus Remote Write Receiver 👋

Howdy!

Perhaps we should chat a bit. I want to use the remote write receiver as part of our managed collector framework (cardinalhq.io for reference) where we want to make it easy to migrate from a prometheus shop to an Otel data flow. We are adding scraping configs, but that seems less useful than just getting the firehose from prometheus directly.

Let's chat and see if we can help develop the receiver.

You can also ping me on slack/git if any doubt arises. I hope that in the next weeks we will have the prw2 working...

#### Description This is an alternative PR to #12825. I'm taking all the commits from that PR and adding the feature gate on the client side, as requested by reviews. The server behavior of peeking into the initial bytes to identify the encoding is kept :) If you've reviewed the previous PR, you can just review the latest commit!  #### Link to tracking issue Fixes #10584 --------- Signed-off-by: Arthur Silva Sens <[email protected]> Co-authored-by: Michael Graff <[email protected]> Co-authored-by: Alex Boten <[email protected]> Co-authored-by: Pablo Baeyens <[email protected]> Co-authored-by: Jonathan <[email protected]>

github-actions · 2025-05-13T03:30:02Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

jade-guiton-dd · 2025-05-13T10:31:47Z

Since #12911 was merged, I think this can be closed.

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [go.opentelemetry.io/collector/component](https://github.com/open-telemetry/opentelemetry-collector) | require | minor | `v1.31.0` -> `v1.32.0` | | [go.opentelemetry.io/collector/component/componenttest](https://github.com/open-telemetry/opentelemetry-collector) | require | minor | `v0.125.0` -> `v0.126.0` | | [go.opentelemetry.io/collector/confmap](https://github.com/open-telemetry/opentelemetry-collector) | require | minor | `v1.31.0` -> `v1.32.0` | | [go.opentelemetry.io/collector/consumer](https://github.com/open-telemetry/opentelemetry-collector) | require | minor | `v1.31.0` -> `v1.32.0` | | [go.opentelemetry.io/collector/consumer/consumertest](https://github.com/open-telemetry/opentelemetry-collector) | require | minor | `v0.125.0` -> `v0.126.0` | | [go.opentelemetry.io/collector/pdata](https://github.com/open-telemetry/opentelemetry-collector) | require | minor | `v1.31.0` -> `v1.32.0` | | [go.opentelemetry.io/collector/processor](https://github.com/open-telemetry/opentelemetry-collector) | require | minor | `v1.31.0` -> `v1.32.0` | | [go.opentelemetry.io/collector/processor/processortest](https://github.com/open-telemetry/opentelemetry-collector) | require | minor | `v0.125.0` -> `v0.126.0` | --- ### Release Notes <details> <summary>open-telemetry/opentelemetry-collector (go.opentelemetry.io/collector/component)</summary> ### [`v1.32.0`](https://github.com/open-telemetry/opentelemetry-collector/blob/HEAD/CHANGELOG.md#v1320v01260) ##### 🛑 Breaking changes 🛑 - `configauth`: Removes deprecated `configauth.Authentication` and `extensionauthtest.NewErrorClient` ([#12992](open-telemetry/opentelemetry-collector#12992)) The following have been removed: - `configauth.Authentication` use `configauth.Config` instead - `extensionauthtest.NewErrorClient` use `extensionauthtest.NewErr` instead ##### 💡 Enhancements 💡 - `service`: Replace `go.opentelemetry.io/collector/semconv` usage with `go.opentelemetry.io/otel/semconv` ([#12991](open-telemetry/opentelemetry-collector#12991)) - `confmap`: Update the behavior of the confmap.enableMergeAppendOption feature gate to merge only component lists. ([#12926](open-telemetry/opentelemetry-collector#12926)) - `service`: Add item count metrics defined in Pipeline Component Telemetry RFC ([#12812](open-telemetry/opentelemetry-collector#12812)) See [Pipeline Component Telemetry RFC](https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/rfcs/component-universal-telemetry.md) for more details: - `otelcol.receiver.produced.items` - `otelcol.processor.consumed.items` - `otelcol.processor.produced.items` - `otelcol.connector.consumed.items` - `otelcol.connector.produced.items` - `otelcol.exporter.consumed.items` - `tls`: Add trusted platform module (TPM) support to TLS authentication. ([#12801](open-telemetry/opentelemetry-collector#12801)) Now the TLS allows the use of TPM for loading private keys (e.g. in TSS2 format). ##### 🧰 Bug fixes 🧰 - `exporterhelper`: Add validation error for batch config if min_size is greater than queue_size. ([#12948](open-telemetry/opentelemetry-collector#12948)) - `telemetry`: Allocate less memory per component when OTLP exporting of logs is disabled ([#13014](open-telemetry/opentelemetry-collector#13014)) - `confmap`: Use reflect.DeepEqual to avoid panic when confmap.enableMergeAppendOption feature gate is enabled. ([#12932](open-telemetry/opentelemetry-collector#12932)) - `internal telemetry`: Add resource attributes from telemetry.resource to the logger ([#12582](open-telemetry/opentelemetry-collector#12582)) Resource attributes from telemetry.resource were not added to the internal console logs. Now, they are added to the logger as part of the "resource" field. - `confighttp and configcompression`: Fix handling of `snappy` content-encoding in a backwards-compatible way ([#10584](open-telemetry/opentelemetry-collector#10584), [#12825](open-telemetry/opentelemetry-collector#12825)) The collector used the Snappy compression type of "framed" to handle the HTTP content-encoding "snappy". However, this encoding is typically used to indicate the "block" compression variant of "snappy". This change allows the collector to: - When receiving a request with encoding 'snappy', the server endpoints will peek at the first bytes of the payload to determine if it is "framed" or "block" snappy, and will decompress accordingly. This is a backwards-compatible change. If the feature-gate "confighttp.framedSnappy" is enabled, you'll see new behavior for both client and server: - Client compression type "snappy" will now compress to the "block" variant of snappy instead of "framed". Client compression type "x-snappy-framed" will now compress to the "framed" variant of snappy. - Servers will accept both "snappy" and "x-snappy-framed" as valid content-encodings. - `tlsconfig`: Disable TPM tests on MacOS/Darwin ([#12964](open-telemetry/opentelemetry-collector#12964))  </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://gitea.t000-n.de/t.behrendt/tracebasedlogsampler/pulls/13 Co-authored-by: Renovate Bot <[email protected]> Co-committed-by: Renovate Bot <[email protected]>

skandragon added 3 commits April 11, 2025 01:08

detect snappy compression type

e3698d8

fully add x-snappy-framed

81cb6fe

add test for snappy-but-framed-body

dd4f97d

skandragon requested a review from a team as a code owner April 11, 2025 06:52

skandragon requested a review from jade-guiton-dd April 11, 2025 06:52

skandragon and others added 4 commits April 11, 2025 04:54

address lint issue

a73dd08

Merge branch 'main' into snappy-done-right

4ecfa24

add changelog

7621d8c

lint-again

db48f79

bogdandrutu requested changes Apr 11, 2025

View reviewed changes

bogdandrutu reviewed Apr 11, 2025

View reviewed changes

ArthurSens mentioned this pull request Apr 15, 2025

Testing changes from collector-core#12825 open-telemetry/opentelemetry-collector-contrib#39446

Closed

ArthurSens reviewed Apr 15, 2025

View reviewed changes

perebaj reviewed Apr 21, 2025

View reviewed changes

perebaj mentioned this pull request Apr 21, 2025

[prometheuswritereceiver] target info open-telemetry/opentelemetry-collector-contrib#38812

Merged

ArthurSens mentioned this pull request Apr 22, 2025

Fix snappy/framed-snappy encoding/decoding #12911

Merged

github-actions bot added the Stale label May 13, 2025

jade-guiton-dd closed this May 13, 2025

Proposed "snappy" vs "x-snappy-framed" content-type confusion #12825

Proposed "snappy" vs "x-snappy-framed" content-type confusion #12825

Uh oh!

Conversation

skandragon commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Link to tracking issue

Testing

Uh oh!

codecov bot commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dmathieu commented Apr 11, 2025

Uh oh!

bogdandrutu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jade-guiton-dd Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skandragon commented Apr 11, 2025

Uh oh!

ArthurSens commented Apr 11, 2025

Uh oh!

ArthurSens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jade-guiton-dd commented Apr 16, 2025

Uh oh!

bogdandrutu commented Apr 17, 2025

Uh oh!

jade-guiton-dd commented Apr 18, 2025

Uh oh!

perebaj left a comment

Choose a reason for hiding this comment

Uh oh!

skandragon commented Apr 28, 2025

Uh oh!

ArthurSens commented Apr 28, 2025

Uh oh!

perebaj commented Apr 28, 2025

Uh oh!

github-actions bot commented May 13, 2025

Uh oh!

jade-guiton-dd commented May 13, 2025

Uh oh!

Uh oh!

skandragon commented Apr 11, 2025 •

edited

Loading

codecov bot commented Apr 11, 2025 •

edited

Loading

jade-guiton-dd Apr 14, 2025 •

edited

Loading